refactor: remove normalModule.loaders #11780
Conversation
jerrykingxyz
requested review from
h-a-n-a,
ahabhgk,
LingyuCoder,
JSerFeng and
quininer
as code owners
github-actions
bot
added
release: refactor
team
✅ Deploy Preview for rspack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to remove the normalModule.loaders field from the NormalModule structure and move loader handling to a more appropriate location. The primary purpose is to decouple loader information from individual modules and centralize it in the build context.
Key changes:
- Remove
loadersfield fromNormalModuleand related loader access methods - Move loaders to
BuildContextandModuleFactoryResultstructures - Remove
rspack_cacheableannotations and dependencies from loader-related code - Update module creation patterns to use the new
ModuleFactoryResult::new_with_modulemethod
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rspack_core/src/normal_module.rs | Removes loaders field and methods from NormalModule, updates build process to use build_context.loaders |
| crates/rspack_core/src/module_factory.rs | Adds loaders field to ModuleFactoryResult and updates factory trait |
| crates/rspack_core/src/normal_module_factory.rs | Updates module creation to populate loaders in ModuleFactoryResult |
| crates/rspack_core/src/module.rs | Adds loaders field to BuildContext structure |
| crates/rspack_loader_*/src/lib.rs | Removes cacheable annotations from loader implementations |
| crates/rspack_plugin_/src/.rs | Updates module creation to use new factory result pattern |
| crates/rspack_binding_api/src/modules/normal_module.rs | Removes loaders exposure from JavaScript bindings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| #[async_trait::async_trait] | ||
| pub trait ModuleFactory: Debug + Sync + Send { | ||
| pub trait ModuleFactory: std::fmt::Debug + Sync + Send { |
There was a problem hiding this comment.
[nitpick] The trait bound should use the standard Debug import instead of the fully qualified path std::fmt::Debug. Since derive_more::Debug is imported at the top, consider using just Debug for consistency with the rest of the codebase.
| pub trait ModuleFactory: std::fmt::Debug + Sync + Send { | |
| pub trait ModuleFactory: Debug + Sync + Send { |
Copilot uses AI. Check for mistakes.
| Ok(Some(ModuleFactoryResult { | ||
| module: Some(module), | ||
| loaders, | ||
| })) |
There was a problem hiding this comment.
[nitpick] This manual struct construction could be replaced with a more explicit constructor method. Consider adding a new_with_module_and_loaders method to ModuleFactoryResult for better API consistency.
| Ok(Some(ModuleFactoryResult { | |
| module: Some(module), | |
| loaders, | |
| })) | |
| Ok(Some(ModuleFactoryResult::new_with_module_and_loaders(Some(module), loaders))) |
Copilot uses AI. Check for mistakes.
📦 Binary Size-limit
🎉 Size decreased by 25.00KB from 47.89MB to 47.86MB (⬇️0.05%) |
CodSpeed Performance ReportMerging #11780 will improve performances by 21.38%Comparing Summary
Benchmarks breakdown
Footnotes |
Summary
Related links
Checklist